Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix #50926: add setting and option for MP3 bitrate #2993

Merged
merged 1 commit into from
Feb 17, 2017

Conversation

Jojo-Schmitz
Copy link
Contributor

@Jojo-Schmitz Jojo-Schmitz commented Feb 12, 2017

and some code (style) and UI cleanup

We may want this in master and 2.1. Not sure it'd apply cleanly to 2.1, but am willing to create a separate PR for that purpose if need be.
Adjusting the handbook would need to be done too, in at least 3 places (Preferences, including a new picture, File formats/MP3 Export and Command line options)

@@ -201,7 +199,8 @@ void Preferences::init()
#else
nativeDialogs = false; // don't use system native file dialogs
#endif
exportAudioSampleRate = exportAudioSampleRates[0];
exportAudioSampleRate = 44100;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's wrong with exportAudioSampleRates[0] ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I deleted it, it was superfluos and duplicated from the pull down menu

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not as superfluos as I though though...

@@ -501,14 +501,15 @@ void Preferences::read()
dir.mkpath(myImagesPath);
dir.mkpath(myTemplatesPath);
dir.mkpath(myPluginsPath);
foreach (QString path, mySoundfontsPath.split(";"))
for (QString path: mySoundfontsPath.split(";"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you miss a space here before :
I'm pretty sure all these space modification will not apply to 2.1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, got rid of those, for now

@@ -720,7 +716,7 @@ void PreferenceDialog::hideEvent(QHideEvent* ev)

void PreferenceDialog::recordButtonClicked(int val)
{
foreach(QAbstractButton* b, recordButtons->buttons()) {
for (QAbstractButton* b: recordButtons->buttons()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idem

@@ -882,7 +878,7 @@ void PreferenceDialog::updateValues()
//
qDeleteAll(localShortcuts);
localShortcuts.clear();
foreach(const Shortcut* s, Shortcut::shortcuts())
for (const Shortcut* s: Shortcut::shortcuts())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idem

if (idx == n) // if not found in table
idx = 0;
exportAudioSampleRate->setCurrentIndex(idx);
index = exportAudioSampleRate->findText(QString("%1").arg(prefs.exportAudioSampleRate));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to make sure that the text is not translated, or better set Data and use findData

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm affraid I don't get it. It uses the same method as alsaSampleRate and alsaPeriodSize use too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will have another look but I guess these two need to be changed as well. If we save the text from the combo box in prefs.exportAudioSampleRate it will be translated. If the translation is different and the user changes his language, then when reading it back it will not work well. That's why other part of the code use the userData of the combobox item and not the text.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're most probably right, I wondered too about translation effects.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, seems I can't get it to work :-(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to work now

@Jojo-Schmitz Jojo-Schmitz force-pushed the mp3-export branch 3 times, most recently from 7008773 to fbcb7bd Compare February 16, 2017 12:40
@lasconic lasconic merged commit 2c8c6db into musescore:master Feb 17, 2017
@Jojo-Schmitz Jojo-Schmitz deleted the mp3-export branch February 17, 2017 20:46
@Jojo-Schmitz
Copy link
Contributor Author

Can we get this into 2.1 too?

@lasconic
Copy link
Contributor

sure, consider it done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants